Remove dependency on system binary from ping input plugin #6050

glinton · 2019-06-27T00:39:46Z

This adds native ping capabilities to the ping input plugin, removing the dependency on a system installed ping. This also allows ping to work on non-english hosts (a downside to the previous version where the output of a system command was being parsed).

Resolves #2833

danielnelson · 2019-07-02T21:24:57Z

plugins/inputs/ping/ping.go

 	Arguments []string

+	// Whether to resolve addresses using ipv6 or not.
+	IPv6 bool
+
 	// host ping function
 	pingHost HostPinger


Is this functionality still used? If not, clean this up and remove. That said, in my mind it would probably be best to turn this into an interface that can be implemented by both the exec code and the native code. This reduces the need to deal with branches and gives a nice "fold" to fake data for unit tests, making testing easier.

I'm not certain I follow. This is used to use an ipv6 address a hostname resolves to. It's similar to the current exec version doing a ping -6 or specifying binary = ping6. It's there as a way to ensure only ipv6 is used.

Comment was attempting to refer to the pingHost field.

danielnelson · 2019-07-02T21:25:53Z

plugins/inputs/ping/ping.go

+			return nil
+		}
+
+		if len(p.Arguments) == 0 && p.Method == "native" {


I think we should use the native ping when method = "native", not taking into account p.Arguments.

danielnelson · 2019-07-02T21:27:14Z

plugins/inputs/ping/ping.go

-	if len(p.Arguments) > 0 {
-		return append(p.Arguments, url)
+func (p *Ping) pingToURLNative(destination string, acc telegraf.Accumulator) {
+	defer p.wg.Done()


Put the defer in the function that calls this function, this is something I always do to avoid splitting the "are we in a goroutine or not" logic across functions.

danielnelson · 2019-07-02T21:38:24Z

plugins/inputs/ping/ping.go

+					Seq: seq,
+				})
+				if err != nil {
+					// likely a timeout error, ignore


Make sure that it's a timeout error, we will want to log anything else. In my testing, I initially got error listening for ICMP packets: listen ip4:icmp : socket: operation not permitted: socket: permission denied because I can't be bothered to read the docs.

Also, it seems that ping returns an error result if any ping times out, so we probably want to do something similar.

danielnelson · 2019-07-02T22:39:26Z

plugins/inputs/ping/ping.go

-			if err != nil {
-				return trans, recv, ttl, min, avg, max, stddev, err
-			}
+	resps := make(chan *ping.Response, chanLength)


Since you read this in a goroutine, you can make the channel much smaller, even unbuffered. Alternately, since we are expecting a low number of concurrent requests, if you size the channel and skip the goroutine moving it after the send loop.

danielnelson · 2019-07-02T22:52:57Z

plugins/inputs/ping/ping.go

-	if err != nil {
-		return min, avg, max, stddev, err
+	stdDev := time.Duration(math.Sqrt(float64(sumsquares / time.Duration(packetsRcvd))))
+	if ttl > 0 {


These tests are a bit strange, why don't we just send all the values for ttl, min, max, average, stddev? I'm guessing this mirrors the exec ping, but I think we can skip it here. In exec ping this indicates that the summary stats couldn't be parsed.

Yes, it was to mirror the exec ping, if they were 0, it would likely have returned before this. Does it make sense to keep checking and prevent ttl from showing up in windows (it will be 0 until it's supported).

I think in the original code we returned these stats from a function as non-pointer values, and lose the ability to tell the difference between "don't know" and 0. This seems like a minor bug in the old code.

In general, when we don't know what one of the values is we don't want to set it and if we do know, even if its zero, we should. Here I think we have 2 cases: TTL doesn't work on Windows and TTL is legitimately 0 (not sure if this is possible, maybe it can only go to 1, but doesn't actually matter).

If the TTL isn't supported in Windows, we should simply check if we are on Windows and not report TTL. This is a more direct way of dealing with shortcoming that are platform specific; add a comment that TTL is not supported on Windows with the go icmp package.

If the TTL is zero, I think we just report it, same goes for min/max/mean/stddev, zero seems valid though unlikely for all of these.

danielnelson · 2019-07-02T22:54:07Z

plugins/inputs/ping/ping.go

 			Binary:       "ping",
 			Arguments:    []string{},
+			ctx:          context.Background(),


Boo for ctx on struct... but is this even used?

Nope, looks like a relic.

danielnelson · 2019-07-02T22:57:13Z

plugins/inputs/ping/ping_test.go

@@ -339,3 +339,20 @@ func TestPingBinary(t *testing.T) {
 	}
 	acc.GatherError(p.Gather)
 }
+
+// Test that Gather function works using native ping
+func TestPingGatherNative(t *testing.T) {


Ideally we could fake the data from the pinger, which would allow better test coverage without hitting external services.

danielnelson · 2019-07-02T23:01:04Z

plugins/inputs/ping/ping_test.go

+
+	var acc testutil.Accumulator
+	p := Ping{
+		Urls:   []string{"www.google.com", "www.reddit.com"},


I think here we should only use "example.org" or "127.0.0.1"

danielnelson · 2019-07-03T00:01:57Z

plugins/inputs/ping/ping.go

+	for p.Count <= 0 || packetsSent < p.Count {
+		select {
+		case <-ctx.Done():
+			goto finish


Just an observation, you don't need to change anything... To me this is an indication flag that this function is too big. If you split it up in the right spots I think error handling could be easier, less variable name aliasing, and no more ugly goto.

yes.. I'd love any pointers if you find yourself with some time

AlirieGray and others added 9 commits April 30, 2019 11:53

WIP: add native ping

ad024c6

Attempt to record accurate ping time

2d173e2

Conditionally use system ping

f097abb

WIP

f2ec8e4

Update go-ping

14e7794

Enforce timeout

f319653

Merge branch 'master' into feat/native-ping-new

ace14ac

Use updated library interface

0272ec1

Update dependency

42198df

glinton added the feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin label Jun 27, 2019

glinton mentioned this pull request Jun 27, 2019

Add separate ping_native input plugin #6007

Closed

glinton added 5 commits June 27, 2019 10:16

Update method logic and add test

2317bbb

fmt

b734cab

Update readme

6563913

Merge branch 'master' into feat/native-ping-new

701c744

Fix typos in readme

05b0e4a

danielnelson suggested changes Jul 3, 2019

View reviewed changes

glinton added 3 commits July 3, 2019 16:08

Address feedback

4a1dddf

Set ttl only on supported platforms

ecdf5f2

fmt tests

a5808b4

danielnelson approved these changes Jul 11, 2019

View reviewed changes

danielnelson merged commit ea6b398 into master Jul 11, 2019

danielnelson deleted the feat/2833 branch July 11, 2019 22:08

danielnelson added this to the 1.12.0 milestone Jul 11, 2019

bitcharmer pushed a commit to bitcharmer/telegraf that referenced this pull request Oct 18, 2019

Add native Go ping method to ping input plugin (influxdata#6050)

1e2bded

athoune pushed a commit to bearstech/telegraf that referenced this pull request Apr 17, 2020

Add native Go ping method to ping input plugin (influxdata#6050)

d584517

idohalevi pushed a commit to idohalevi/telegraf that referenced this pull request Sep 29, 2020

Add native Go ping method to ping input plugin (influxdata#6050)

20c15f1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove dependency on system binary from ping input plugin #6050

Remove dependency on system binary from ping input plugin #6050

glinton commented Jun 27, 2019

danielnelson Jul 2, 2019

glinton Jul 3, 2019

danielnelson Jul 3, 2019

danielnelson Jul 2, 2019

danielnelson Jul 2, 2019

danielnelson Jul 2, 2019

danielnelson Jul 2, 2019

danielnelson Jul 2, 2019

glinton Jul 3, 2019

danielnelson Jul 3, 2019

danielnelson Jul 2, 2019

glinton Jul 3, 2019

danielnelson Jul 2, 2019

danielnelson Jul 2, 2019

danielnelson Jul 3, 2019

glinton Jul 3, 2019

Remove dependency on system binary from ping input plugin #6050

Remove dependency on system binary from ping input plugin #6050

Conversation

glinton commented Jun 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment